feat(spp_api_v2): support HTTP Basic Auth on OAuth token endpoint#69
feat(spp_api_v2): support HTTP Basic Auth on OAuth token endpoint#69
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the OAuth token endpoint by adding support for RFC 6749-compliant client authentication methods, including HTTP Basic Auth and form-encoded bodies, alongside the existing JSON body. It also introduces a configurable token lifetime, allowing administrators to adjust the duration of issued access tokens. These changes are crucial for integrating with clients like QGIS that rely on standard OAuth2 authentication flows. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for HTTP Basic Authentication and form-encoded bodies to the OAuth token endpoint, which is a great enhancement for RFC compliance and QGIS integration. The implementation looks mostly good, but I have a few suggestions to improve robustness, maintainability, and test coverage. Specifically, I've pointed out some areas where error handling could be improved by adding logging instead of silently passing exceptions, a piece of duplicated code that could be refactored, and a local import that should be moved. Most importantly, the new authentication methods are not covered by automated tests, which is a critical gap to fill before merging.
spp_api_v2/routers/oauth.py
Outdated
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Catching a broad Exception and silently passing is a risky pattern as it can hide unexpected errors and make debugging difficult. While the intent is to fall back to other authentication methods, it's better to at least log the exception to have visibility into why JSON parsing might be failing. This could be due to malformed JSON, a client sending an incorrect data structure, or another unexpected issue.
| except Exception: | |
| pass | |
| except Exception as e: | |
| _logger.debug("Could not parse token request from JSON body, falling back: %s", e) | |
| pass |
| self.assertEqual(data["token_type"], "Bearer") | ||
| self.assertIn("expires_in", data) | ||
| self.assertEqual(data["expires_in"], 3600) # 1 hour | ||
| self.assertEqual(data["expires_in"], 86400) # 24 hours (default) |
There was a problem hiding this comment.
While this test is correctly updated for the new default token lifetime, the test suite is missing coverage for the new authentication methods introduced in this PR (HTTP Basic Auth and application/x-www-form-urlencoded body). Adding tests for these new code paths is crucial to ensure they work as expected and to prevent future regressions.
spp_api_v2/routers/oauth.py
Outdated
| basic_client_secret = "" | ||
| auth_header = http_request.headers.get("authorization", "") | ||
| if auth_header.startswith("Basic "): | ||
| import base64 |
There was a problem hiding this comment.
According to PEP 8, imports should usually be at the top of the file. Moving this import statement to the top of the file with other imports will improve readability and consistency.
spp_api_v2/routers/oauth.py
Outdated
| except (ValueError, UnicodeDecodeError): | ||
| pass |
There was a problem hiding this comment.
Silently ignoring ValueError or UnicodeDecodeError when decoding the Basic Auth header can make it difficult to debug issues with client requests. It would be beneficial to log these exceptions at a debug level to aid in troubleshooting malformed Authorization headers.
| except (ValueError, UnicodeDecodeError): | |
| pass | |
| except (ValueError, UnicodeDecodeError) as e: | |
| _logger.debug("Failed to decode Basic Auth header: %s", e) | |
| pass |
spp_api_v2/routers/oauth.py
Outdated
| # Read configurable token lifetime (default: 24 hours) | ||
| # nosemgrep: odoo-sudo-without-context | ||
| token_lifetime_hours = int(env["ir.config_parameter"].sudo().get_param("spp_api_v2.token_lifetime_hours", "24")) |
There was a problem hiding this comment.
The logic to retrieve token_lifetime_hours from the configuration is duplicated here and in the get_token function (lines 142-144). This duplication leads to an extra database query and makes the code harder to maintain. Consider refactoring to fetch this value only once in get_token and then pass it as an argument to _generate_jwt_token.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #69 +/- ##
==========================================
+ Coverage 55.79% 61.90% +6.11%
==========================================
Files 162 373 +211
Lines 9291 20561 +11270
==========================================
+ Hits 5184 12729 +7545
- Misses 4107 7832 +3725
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| body = await http_request.json() | ||
| return TokenRequest(**body) | ||
| except Exception as e: | ||
| _logger.debug("Could not parse token request from JSON body, falling back: %s", e) |
Check warning
Code scanning / Semgrep OSS
Semgrep Finding: python.lang.security.audit.logging.logger-credential-leak.python-logger-credential-disclosure Warning
Add RFC 6749-compliant client authentication to /oauth/token endpoint: - HTTP Basic Auth header parsing (Authorization: Basic) - Form-encoded body support (application/x-www-form-urlencoded) - Configurable token lifetime via spp_api_v2.token_lifetime_hours (default 24h) - Keeps JSON body for backwards compatibility Needed for QGIS native OAuth2 flow, which sends Basic Auth headers.
- Move import base64 to top-level imports - Add debug logging to Basic Auth decode errors - Add debug logging to broad except in JSON body parsing - Remove duplicate token_lifetime_hours fetch from _generate_jwt_token by passing it as a parameter - Add tests for HTTP Basic Auth and form-encoded body authentication
jeremi
force-pushed
the
feat/api-v2-oauth-basic-auth
branch
from
a37283c to
355db0e
Compare
1 participant
Summary
/oauth/tokenendpointAuthorization: Basic base64(id:secret))application/x-www-form-urlencoded) alongside JSONspp_api_v2.token_lifetime_hourssystem parameter (default 24h)Why: Needed for QGIS native OAuth2 flow, which sends Basic Auth headers per RFC 6749 Section 2.3.1.
Test plan
expires_inexpectation from 3600 to 86400)./scripts/test_single_module.sh spp_api_v2